Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add default units for all dimensions #3143

Merged
merged 3 commits into from
Nov 19, 2022

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Nov 5, 2022

Fixes #3150
Fixes #3121
Fixes #3096

It would be great if someone from the US could check if I did pick good units for the imperial measurement system or if something is missing there.

Signed-off-by: Jan N. Klug [email protected]

@J-N-K J-N-K added the bug An unexpected problem or unintended behavior of the Core label Nov 5, 2022
@J-N-K J-N-K requested a review from a team as a code owner November 5, 2022 20:25
Signed-off-by: Jan N. Klug <[email protected]>
@rkoshak
Copy link

rkoshak commented Nov 7, 2022

Use "Feet" for length unless having the default units close to the same size between SI and Imperial is important. If so use "Yard" which is closest to a meter in length.

What I don't know is if it makes sense to provide imperial units for some of the more scientific units like acceleration, density, and the like.

Everything else looks good.

@mstormi mstormi mentioned this pull request Nov 8, 2022
Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K
Copy link
Member Author

J-N-K commented Nov 13, 2022

@rkoshak For acceleration it probably makes sense, because we also have different units for speed. I'm not so sure about density, my guess would be that the most common use-case here is absolute humidity. What is the usual unit to express that?

@rkoshak
Copy link

rkoshak commented Nov 14, 2022

In all my years I've never been in a situation where I've ever used or seen absolute humidity used. It's affected by so many other variables it's not all that useful (IIRC). Beyond that I'm not sure where I've ever seen these in use day-to-day. Maybe in measuring how full a propane tank. I did some looking and it looks like pounds per cubic foot is most commonly used.

I'm not sure what makes sense for acceleration. Since feet is the base unit we use I guess feet per second squared would make sense as a default here. I can't imagine in a home automation context where we'd care about acceleration in yards or miles per second squared by default.

I've only ever encountered this sort of stuff in math and physics classes though and we used SI units even twenty-five years ago when I last took them.

@J-N-K
Copy link
Member Author

J-N-K commented Nov 14, 2022

I guess we should leave it as we have it now. BTW: I use absolute humidity to check if it makes sense to use the bathroom fan. If the absolute humidity outside is higher than inside, don't turn it on even if the relative humidity is higher than the set value.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

@kaikreuzer kaikreuzer merged commit d00d14d into openhab:main Nov 19, 2022
@kaikreuzer kaikreuzer added this to the 3.4 milestone Nov 19, 2022
@J-N-K J-N-K deleted the feature-unitprovider branch November 19, 2022 22:14
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/evcc-binding-electric-vehicle-charging-control/135209/19

addDefaultUnit(ElectricInductance.class, Units.HENRY);
addDefaultUnit(ElectricPotential.class, Units.VOLT);
addDefaultUnit(ElectricResistance.class, Units.OHM);
addDefaultUnit(Energy.class, Units.JOULE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be this change causes all kind of channels that are kWh to display in J, which is not expected. Is there a fix possible?

Copy link
Member

@Hilbrand Hilbrand Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this mean %unit% should not be used in binding xml definitions anymore? Or is it a bug in core where if might be that if specified as %unit% the default takes precedence over the unit in the quantity type? (I haven't looked into the source code yet)

Copy link
Member Author

@J-N-K J-N-K Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be. However, there is no way to avoid that and it was always the case with the dimensions that had a default unit before.

The state description takes precedence over the default (and that works), so we could check if we need to adjust %unit%. I'll have a look.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@J-N-K The difference to other dimensions is that this here now is a breaking change - many of my items now show J instead of KWh and even a couple of my rules are impacted. I assume @Hilbrand (and many others) have a similar situation.
I wonder whether we should maybe define kWh as a default for Energy - after all, this is the unit that is almost always used in a smart home context, while J is rather a very rare exception
Wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaikreuzer for the channels with %unit% my issue was solved in #3201.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the J might be the standard unit I think in a home automation context kWhshould absolutely be the default.
99% of the time users want to compare/measure energy consumption of devices and that's typically done in kWh

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/kostal-thirdgeneration-plenticore-plus-10-all-dc-values-are-null/141878/1

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/uom-default-units-and-consequences/142360/67

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
* Add default units for all dimensions

Signed-off-by: Jan N. Klug <[email protected]>
GitOrigin-RevId: d00d14d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core
Projects
None yet
6 participants